Files that changed from the base of the PR and between fb902d2 and 2759bfd.

• CHANGELOG.md (1 hunks)
• integration_tests/test_ica_precompile.py (2 hunks)
• x/cronos/keeper/keeper.go (1 hunks)

x/cronos/keeper/keeper.go (1)

320-322: Verify the impact of this change across the codebase

The new condition for handling unsuccessful underlying app responses addresses the PR objective. To ensure this change doesn't have unintended consequences, it's important to verify its impact across the codebase.

Please run the following script to check for any other occurrences of UnderlyingAppSuccess and ensure they are handled consistently:

This will help ensure that the change is consistent with other parts of the codebase and that appropriate test cases exist.

CHANGELOG.md (1)

27-27: LGTM: New bug fix added to UNRELEASED section

The new line correctly documents a bug fix related to acknowledgement alignment with underlying app success when the acknowledgement packet does not succeed. This is properly linked to the corresponding pull request (#1633).

integration_tests/test_ica_precompile.py (3)

Line range hint 308-337: Confirm that all function calls are updated with the correct parameters

Throughout the changes from lines 308 to 337, ensure that all calls to submit_msgs and related functions use the correct and updated parameters, especially after any refactoring.

Run the following script to check for parameter usage:

#!/bin/bash
# Description: Find all calls to `submit_msgs` and list their parameters.

# Test: Extract `submit_msgs` calls and display the parameters.
rg --type python 'submit_msgs\(' -A 10

---

317-318: ⚠️ Potential issue

Correct indentation and ensure proper assertion placement

Lines 317-318 contain assertion statements that are not indented properly. This could lead to syntax errors or logical issues in the test execution.

Apply this diff to fix the indentation:

-    assert expected_seq == last_seq
-    assert status == Status.FAIL
+        assert expected_seq == last_seq
+        assert status == Status.FAIL

Likely invalid or redundant comment.

---

308-308: Clarify need_wait=False parameter

In line 308, when calling submit_msgs, the parameter need_wait is set to False. This means the function will not wait for the transaction confirmation, which might lead to race conditions or flaky tests if subsequent code depends on the transaction's completion. Ensure that this is intentional and that the test accounts for the asynchronous nature of the call.

Consider verifying the necessity of need_wait=False:

✅ Verification successful

Clarify need_wait=False Parameter

After analyzing the codebase, there are no subsequent assertions or operations that depend on the transaction's confirmation following submit_msgs calls with need_wait=False. This setup appears intentional and the tests account for the asynchronous nature of the call.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if any other parts of the test depend on the transaction confirmation.

# Test: Search for usage of variables that might be affected.
rg --type python 'last_seq|status|balance' -A 5

Length of output: 165

---

Script:

#!/bin/bash
# Description: Verify if other parts of the test depend on the transaction confirmation by searching for relevant variables.

# Corrected rg command with proper file type and syntax
rg --type py 'submit_msgs\(' -A 10

Length of output: 9017

---

Script:

#!/bin/bash
# Description: Identify if there are any assertions or operations following submit_msgs calls that depend on transaction confirmation.

# Search for submit_msgs calls with need_wait=False and check the next few lines for assertions or state-dependent operations.
rg --type py 'submit_msgs\([^)]*need_wait=False[^)]*\)' -A 10

Length of output: 63

Files that changed from the base of the PR and between 2759bfd and cdec1dd.

• integration_tests/test_ica_precompile.py (2 hunks)
• x/cronos/keeper/keeper.go (1 hunks)

integration_tests/test_ica_precompile.py (4)

309-309: LGTM: Improved flexibility in message submission

The addition of need_wait=False allows for more flexible handling of message submission without waiting for confirmation. This change is consistent with the updated submit_msgs function signature and provides better control over the test flow.

---

318-320: LGTM: Improved error handling and log verification

The addition of the assertion assert status == Status.FAIL and the wait_for_packet_log function call significantly improves the test's robustness. These changes ensure that the test properly verifies the failure status and waits for the corresponding log, addressing the previous concern about error handling for unexpected statuses.

---

338-338: LGTM: Improved sequence verification

The addition of assert expected_seq == last_seq is a valuable improvement to the test. This assertion ensures that the expected sequence number matches the last sequence number returned by the contract, verifying the correct sequencing of operations in the ICA functionality.

---

321-338: 🛠️ Refactor suggestion

Consider refactoring repetitive code patterns

While the added code successfully performs the necessary steps for testing ICA functionality, there are still repetitive patterns that could benefit from refactoring. Consider creating a helper function to encapsulate the common operations of funding, submitting messages, and verifying status. This would improve code maintainability and readability.

Example refactoring:

def perform_ica_operation(ibc, tcontract, ica_address, expected_seq, signer):
    balance = funds_ica(ibc.chainmain.cosmos_cli(), ica_address, signer=signer)
    start = ibc.cronos.w3.eth.get_block_number()
    str, diff = submit_msgs(
        ibc,
        tcontract.functions.callSubmitMsgs,
        {"from": ADDRS[signer], "gas": 500000},
        ica_address,
        False,
        expected_seq,
        tcontract.events.SubmitMsgsResult,
        get_next_channel(ibc.cronos.cosmos_cli(), connid),
        signer=signer,
    )
    last_seq = tcontract.caller.getLastSeq()
    wait_for_status_change(tcontract, channel_id, last_seq)
    status = tcontract.caller.getStatus(channel_id, last_seq)
    assert expected_seq == last_seq
    assert status == Status.SUCCESS
    wait_for_packet_log(start, tcontract.events.OnPacketResult, channel_id, last_seq, status)
    return balance - diff

This refactoring would make the test more maintainable and easier to read.